-
-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preparations for removal of CTI/XML #1159
Conversation
e900176
to
0bd0ded
Compare
Codecov Report
@@ Coverage Diff @@
## main #1159 +/- ##
==========================================
- Coverage 66.30% 65.10% -1.20%
==========================================
Files 315 315
Lines 45283 45258 -25
Branches 19237 19240 +3
==========================================
- Hits 30024 29467 -557
- Misses 12644 13329 +685
+ Partials 2615 2462 -153
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on @speth! Just a few small comments here.
@@ -2756,6 +2761,11 @@ def convert(filename=None, outName=None, text=None): | |||
def main(): | |||
if len(sys.argv) not in (2,3): | |||
raise ValueError('Incorrect number of command line arguments.') | |||
print("WARNING: The CTI and XML input file formats are deprecated and will be\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think switching this to DeprecationWarning
would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using raise DeprecationWarning(...)
has the effect of also printing out the stack trace, which most users are not interested in and I think has the effect of causing people to miss the actual important warning text, e.g.:
Traceback (most recent call last):
File "/home/speth/src/cantera/interfaces/cython/cantera/ctml_writer.py", line 2772, in <module>
main()
File "/home/speth/src/cantera/interfaces/cython/cantera/ctml_writer.py", line 2764, in main
raise DeprecationWarning("WARNING: The CTI and XML input file formats are deprecated and will be\n"
DeprecationWarning: WARNING: The CTI and XML input file formats are deprecated and will be
removed in Cantera 3.0. Use `cti2yaml.py` to convert CTI input files to
the YAML format. See https://cantera.org/tutorials/legacy2yaml.html for
more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to raise
the DeprecatuonWarning
. I think importing warnings
and using that interface would be better. https://docs.python.org/3/library/warnings.html#warnings.warn
Perhaps FutureWarning
would be better, too: https://docs.python.org/3/library/exceptions.html#FutureWarning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I was misunderstanding how DeprecationWarning
is supposed to be used, based on the fact that it is a type of Exception
. But the output from warnings.warn
still seems clunkier than I'd prefer:
/home/speth/src/cantera/build/python/cantera/ctml_writer.py:2765: FutureWarning:
The CTI and XML input file formats are deprecated and will be
removed in Cantera 3.0. Use `cti2yaml.py` to convert CTI input files to
the YAML format. See https://cantera.org/tutorials/legacy2yaml.html for
more information.
warnings.warn(
The line reference makes sense, but I don't really get why it prints the line of code that starts the call to warnings.warn
(which, if the message were shorter, would just be repeating the message again).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the recommendation in the docs is to replace warnings.formatwarning
(via SO). Something like
def custom_formatwarnings(msg, category, filename, lineno, line=None):
return f"{filename}:{lineno}:{msg}\n"
warnings.formatwarning = custom_formatwarnings
Since the message is longer though, it doesn't bother me too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what is "better" about using the warnings
interface when:
- This message is triggered only when this file is called as a script (i.e., through the
main
method), so there's no benefit/opportunity for anything to interact with settings that would either suppress this warning or elevate it into an error - All other warning output from this script is just handled by
print
anyway - The entire script is deprecated and I'm planning deleting it as soon as we release Cantera 2.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @allow_deprecated
decorator is quite nice!
Beyond, I have a couple of comments.
src/base/xml.cpp
Outdated
warn_deprecated("XML_Node::build", | ||
"\nThe CTI and XML input file formats are deprecated and will be removed in\n" | ||
"Cantera 3.0. Use `cti2yaml.py` or `ctml2yaml.py` to convert CTI or XML input\n" | ||
"files to the YAML format. See https://cantera.org/tutorials/legacy2yaml.html\n" | ||
"for more information."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be regular single quotes rather than back-ticks? (Here and elsewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a separate note, would it make sense to promote the legacy2yaml
page to a more prominent position on the tutorials page (right now it's within the input-files sub-category)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm starting to wondering if a large chunk of the "Input" documentation needs to be promoted a level in the website -- Right now, it's all buried as a subsection under "tutorials", which has the effect of both putting the input file related tutorials and extra layer deep, and hiding the YAML format documentation inside the tutorials section, which isn't really correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I take that back - Besides being accessible from within the "Tutorials" section, the general YAML documentation is available from the main cantera.org
page under Documentation -> YAML Input File Users' Guide, and the documentation for specific models is available as Documentation -> YAML Input File API Reference, which seems about right.
I'm not sure how much more prominent the legacy2yaml
page needs to be. Within the tutorials section, I think it's in the right place, though I suppose it could come before the deprecated options of "creating a new CTI file" and "conversion from Chemkin to CTI". For users who don't think they need tutorials, an additional link on the "Documentation" page wouldn't hurt either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think that the input format description should be linked on the main page. It’s tangential here, and the url in the warnings is fine
This is necessary so these tests can be retained after the CTI format is removed and only the cti2yaml conversion tool remains.
This is necessary so these tests can be retained after the XML format is removed and only the ctml2yaml conversion tool remains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @speth!
@@ -2756,6 +2761,11 @@ def convert(filename=None, outName=None, text=None): | |||
def main(): | |||
if len(sys.argv) not in (2,3): | |||
raise ValueError('Incorrect number of command line arguments.') | |||
print("WARNING: The CTI and XML input file formats are deprecated and will be\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me!
Changes proposed in this pull request
This PR takes care of changes that are needed to allow clean removal of the CTI and XML formats after the release of Cantera 2.6.
elements.xml
ck2yaml
,cti2yaml
, andctml2yaml
instead of CTI or XML files these tests can be maintained even after the removal of the CTI and XML functionalitycti2yaml
to create an empty Kinetics model when one is implied (for example, by theideal_gas
directive)T-max
for species thermo in XML to YAML conversionsctml_writer
toSI
ThermoPhase::getParameters(int, double*)
andThermoPhase::setParameters(int, double*)
Checklist
scons build
&scons test
) and unit tests address code coverage